-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL block type for exponential histograms #133393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
1a4c296 to
65b3d0e
Compare
bc159b1 to
b991e5b
Compare
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
07c9955 to
8a0a14f
Compare
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/BlockUtils.java
Show resolved
Hide resolved
...l/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlockBuilder.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
x-pack/plugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/RandomBlock.java
Outdated
Show resolved
Hide resolved
.../testFixtures/src/main/java/org/elasticsearch/xpack/esql/JsonBackedExponentialHistogram.java
Outdated
Show resolved
Hide resolved
...l/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlockBuilder.java
Outdated
Show resolved
Hide resolved
...sql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java
Outdated
Show resolved
Hide resolved
...sql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it to the esql experts to approve.
…h/compute/test/RandomBlock.java Co-authored-by: Kostas Krikellas <[email protected]>
I understand - we should reuse the existing DoublesBlockLoader/IntsBlockLoader to read sub-blocks. Similarly, for the histogram block, there will be at least two blocks: one for zeroThreshold and one for the buckets. The buckets block will be loaded using BytesRefsFromCustomBinary, and zeroThreshold can be loaded with LongsBlockLoader. This approach avoids combining these values during reading. When accessing the histogram value, we can retrieve zeroThreshold and decode the encoded histogram BytesRef from the buckets block.
Just to confirm - you prefer to experiment with our discussion in follow-up PRs. If so, I'll need to take another look to make sure we don’t break existing things. |
This reverts commit 8a3394b.
This reverts commit 9af71b4.
# Conflicts: # benchmarks/src/main/java/org/elasticsearch/benchmark/exponentialhistogram/ExponentialHistogramMergeBench.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/mapper-exponential-histogram/src/main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
14d8614 to
f491a55
Compare
f491a55 to
621b472
Compare
Adds a barebones ES|QL and block type for exponential histograms.
To keep this PR as small as possible I've reduced the type to just storing thescaleof a histogram and nothing else.Everything else will be added in follow-up PRs.
Update: To better get the bigger picture, the block has been now fully fleshed out with all sub-blocks in this PR.
In this PR I've marked the shortcuts / disabled tests that definitely need work before we eventually can put this into tech-preview with
TODO(b/133393).Note that I've also excluded the type from some tests (e.g. TopN) without a
TODO(b/133393): These are tests which cover functionality which I think won't be needed, at least for a tech-preview. Please carefully review them and let me know if those cover functionality which should work, so that I can add theTODO(b/133393)there aswell.Initally this PR also included some CSV-tests. But I decided to remove them for now, as they would require implementing a blockloader, increasing the size of the PR unnecessarily. I'll add them back together with the blockloader in a followup PR.